Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[PWGCF,PWGDQ] Weight merging #9627

Merged
merged 10 commits into from
Feb 4, 2025
Merged

[PWGCF,PWGDQ] Weight merging #9627

merged 10 commits into from
Feb 4, 2025
+381 −282

Conversation

EmilGorm
Copy link
Contributor

@EmilGorm EmilGorm commented Jan 30, 2025 •
edited
Loading

Attempt to fix Hyperloop-only merge issue for dynamically added GFWWeights

  • Added Merge function to GFWWeightsList
  • Added GFWWeightsList as output in flowGenericFramework task, replaces GFWWeights stored in TList
  • Modified input corrections to read GFWWeightsList over Tlist
  • Both GFWWeightsList and GFWWeights are fully backwards compatible aside from the change to camelCase

O2 linter:

  • A persistent request to change to camelCase for function names clashes with ROOTs naming convention for the Merge function
  • Otherwise I have followed the suggestions, but as multiple analysis tasks use the GFWWeights, I had to update the calls to GFWWeights to reflect the O2 Linter camelCase suggestions
  • Remaining O2 Linter suggestions will be the individual users' responsibility (Similar for remaining Mega Linter)

@github-actions github-actions bot changed the title Weight merging [PWGCF,PWGDQ] Weight merging Jan 30, 2025
@EmilGorm EmilGorm marked this pull request as ready for review January 30, 2025 14:23
@alibuild
Copy link
Collaborator

alibuild commented Jan 30, 2025 •
edited
Loading

Error while checking build/O2Physics/o2 for a9cc77a at 2025-01-31 10:12:

## sw/BUILD/O2Physics-latest/log
/sw/SOURCES/O2Physics/9627-slc9_x86-64/0/PWGCF/Flow/Tasks/flowRunbyRun.cxx:322:21: error: 'class GFWWeights' has no member named 'Fill'; did you mean 'fill'?
/sw/SOURCES/O2Physics/9627-slc9_x86-64/0/PWGCF/Flow/Tasks/flowGfwTask.cxx:335:17: error: 'class GFWWeights' has no member named 'SetPtBins'; did you mean 'setPtBins'?
/sw/SOURCES/O2Physics/9627-slc9_x86-64/0/PWGCF/Flow/Tasks/flowGfwTask.cxx:336:17: error: 'class GFWWeights' has no member named 'Init'; did you mean 'init'?
/sw/SOURCES/O2Physics/9627-slc9_x86-64/0/PWGCF/Flow/Tasks/flowGfwTask.cxx:480:33: error: 'class GFWWeights' has no member named 'GetNUA'; did you mean 'getNUA'?
/sw/SOURCES/O2Physics/9627-slc9_x86-64/0/PWGCF/Flow/Tasks/flowGfwTask.cxx:719:19: error: 'class GFWWeights' has no member named 'Fill'; did you mean 'fill'?
/sw/SOURCES/O2Physics/9627-slc9_x86-64/0/PWGCF/Flow/Tasks/flowSP.cxx:189:17: error: 'class GFWWeights' has no member named 'SetPtBins'; did you mean 'setPtBins'?
/sw/SOURCES/O2Physics/9627-slc9_x86-64/0/PWGCF/Flow/Tasks/flowSP.cxx:190:17: error: 'class GFWWeights' has no member named 'Init'; did you mean 'init'?
/sw/SOURCES/O2Physics/9627-slc9_x86-64/0/PWGCF/Flow/Tasks/flowSP.cxx:192:20: error: 'class GFWWeights' has no member named 'SetPtBins'; did you mean 'setPtBins'?
/sw/SOURCES/O2Physics/9627-slc9_x86-64/0/PWGCF/Flow/Tasks/flowSP.cxx:193:20: error: 'class GFWWeights' has no member named 'Init'; did you mean 'init'?
/sw/SOURCES/O2Physics/9627-slc9_x86-64/0/PWGCF/Flow/Tasks/flowSP.cxx:195:20: error: 'class GFWWeights' has no member named 'SetPtBins'; did you mean 'setPtBins'?
/sw/SOURCES/O2Physics/9627-slc9_x86-64/0/PWGCF/Flow/Tasks/flowSP.cxx:196:20: error: 'class GFWWeights' has no member named 'Init'; did you mean 'init'?
/sw/SOURCES/O2Physics/9627-slc9_x86-64/0/PWGCF/Flow/Tasks/flowSP.cxx:413:42: error: 'class GFWWeights' has no member named 'GetNUA'; did you mean 'getNUA'?
/sw/SOURCES/O2Physics/9627-slc9_x86-64/0/PWGCF/Flow/Tasks/flowSP.cxx:763:21: error: 'class GFWWeights' has no member named 'Fill'; did you mean 'fill'?
/sw/SOURCES/O2Physics/9627-slc9_x86-64/0/PWGCF/Flow/Tasks/flowSP.cxx:766:26: error: 'class GFWWeights' has no member named 'Fill'; did you mean 'fill'?
/sw/SOURCES/O2Physics/9627-slc9_x86-64/0/PWGCF/Flow/Tasks/flowSP.cxx:769:26: error: 'class GFWWeights' has no member named 'Fill'; did you mean 'fill'?
ninja: build stopped: subcommand failed.

Full log here.

@victor-gonzalez
Copy link
Collaborator

victor-gonzalez commented Jan 30, 2025 •
edited
Loading

Hi Emil, thanks for doing this!!!
Be careful
One linter recommendation is really mandatory because it actually has dangerous effects if not followed
In fact, the code in FlowGFWPbPb.cxx is working in a not expected way
I'm referring to incorporate std:: before abs
The behavior of abs, alone, has changed a while ago in the grid machines. Now it always round the argument to the previous integer, which is not expected. With the std:: prefix it works as expected, integer when the argument is integer and float when the argument is float

@EmilGorm
Copy link
Contributor Author

Hi Victor,
Thanks for the comment. Good to know about the abs, can I ask, does that issue also exist for the nabs() commonly used in the filters?

@victor-gonzalez
Copy link
Collaborator

Hi Victor, Thanks for the comment. Good to know about the abs, can I ask, does that issue also exist for the nabs() commonly used in the filters?

No, nabs() behaves correctly

victor-gonzalez
victor-gonzalez approved these changes Jan 31, 2025
View reviewed changes
Copy link
Collaborator

@victor-gonzalez victor-gonzalez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Emil!!

@ilikmeta, the source file FlowGFWPbPb.cxx has an issue with the use of abs without prefix. This source file used to be modified by you but you have created now a different one. Should you dispose the previous FlowGFWPbPb.cxx? If you consider it should stay, the abs issue should be fixed because the effect of its usage is not what is expected

@ilikmeta
Copy link
Contributor

@victor-gonzalez Thanks for pointing this out. In previous PR, abs prefix was not seem to be a problem. In the new version of FlowGFWPbPb.cxx that is named (O2 linter basis) flowGfwTask.cxx this is fixed with all other issues stated by O2 linter.

@victor-gonzalez
Copy link
Collaborator

@victor-gonzalez Thanks for pointing this out. In previous PR, abs prefix was not seem to be a problem. In the new version of FlowGFWPbPb.cxx that is named (O2 linter basis) flowGfwTask.cxx this is fixed with all other issues stated by O2 linter.

OK! Now I understand!
But then, one of the two PR this one, @EmilGorm, or yours, @ilikmeta, will get obsolete
The first which get merged will require from the other to be rebased with the latest changes
Please, be aware
Thanks!

@victor-gonzalez
Copy link
Collaborator

@ilikmeta But wait a second. Your old source file FlowGFWPbPb.cxx is still there. Why?
Could you please get rid of it

victor-gonzalez
victor-gonzalez requested changes Jan 31, 2025
View reviewed changes
Copy link
Collaborator

@victor-gonzalez victor-gonzalez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EmilGorm Sorry about that but this is going to break the tag unless it is rebased to the latest master. @ilikmeta PR is already merged, since this morning, and it is without these changes

@ilikmeta
Copy link
Contributor

@ilikmeta But wait a second. Your old source file FlowGFWPbPb.cxx is still there. Why? Could you please get rid of it

Sorry for the confusion, it was supposed to be renamed instead of pushing a new task.The old task FlowGFWPbPb.cxx will be deleted through the last PR.

@EmilGorm
Copy link
Contributor Author

@EmilGorm Sorry about that but this is going to break the tag unless it is rebased to the latest master. @ilikmeta PR is already merged, since this morning, and it is without these changes

Hi Victor, it should already be rebased against the master in the last commit. At least it says up-to-date when pushing after the rebase.

@victor-gonzalez
Copy link
Collaborator

@EmilGorm Sorry about that but this is going to break the tag unless it is rebased to the latest master. @ilikmeta PR is already merged, since this morning, and it is without these changes

Hi Victor, it should already be rebased against the master in the last commit. At least it says up-to-date when pushing after the rebase.

Not any longer
Please, see the conflict message

victor-gonzalez
victor-gonzalez approved these changes Feb 3, 2025
View reviewed changes
Copy link
Collaborator

@victor-gonzalez victor-gonzalez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!!

@EmilGorm
Copy link
Contributor Author

EmilGorm commented Feb 4, 2025

Thanks for the approval, Victor, I guess I also need DQ approval because of the affected files.

@victor-gonzalez
Copy link
Collaborator

Thanks for the approval, Victor, I guess I also need DQ approval because of the affected files.

Actually, yes!
@iarsene @dsekihat could you please have a look and approve by DQ if you agree
Thanks!

@iarsene iarsene merged commit 26d9dd1 into AliceO2Group:master Feb 4, 2025
12 of 14 checks passed
@EmilGorm EmilGorm deleted the WeightMerging branch February 5, 2025 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@victor-gonzalez victor-gonzalez victor-gonzalez approved these changes

@iarsene iarsene iarsene approved these changes

@alibuild alibuild Awaiting requested review from alibuild alibuild is a code owner

@dsekihat dsekihat Awaiting requested review from dsekihat dsekihat is a code owner

@feisenhu feisenhu Awaiting requested review from feisenhu feisenhu is a code owner

@lucamicheletti93 lucamicheletti93 Awaiting requested review from lucamicheletti93 lucamicheletti93 is a code owner

@saganatt saganatt Awaiting requested review from saganatt saganatt is a code owner

@zchochul zchochul Awaiting requested review from zchochul zchochul is a code owner

@lgraczykCern lgraczykCern Awaiting requested review from lgraczykCern lgraczykCern is a code owner

@prchakra prchakra Awaiting requested review from prchakra prchakra is a code owner

@lauraser lauraser Awaiting requested review from lauraser lauraser is a code owner

@ariedel-cern ariedel-cern Awaiting requested review from ariedel-cern ariedel-cern is a code owner

@otonvd otonvd Awaiting requested review from otonvd otonvd is a code owner

@shouqiye shouqiye Awaiting requested review from shouqiye shouqiye is a code owner

Assignees
No one assigned
Labels
pwgcf pwgdq
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@EmilGorm @alibuild @victor-gonzalez @ilikmeta @iarsene